Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Fix common package to work with node16 module resolution. #627

Merged
merged 1 commit into from
Oct 17, 2024
Merged

fix: Fix common package to work with node16 module resolution. #627

merged 1 commit into from
Oct 17, 2024

Conversation

kinyoklion
Copy link
Member

@kinyoklion kinyoklion commented Oct 16, 2024 •
edited
Loading

When using typescript with moduleResolution set to "node16" types were not loading which are part of the common package.

This relates to changes to the default was in which file resolution occurs.

This PR will address this, but we will want a better long-term fix. In this PR we convince node that the .d.ts files are the correct extension by making a small package.json that sets the module type.

In the future we would want the .d.ts files to instead be .d.cts.

Potentially we can move to tsup: https://tsup.egoist.dev/

But the changes were too extensive for this fix.

Testing:

tsconfig.json

{
  "$schema": "https://json.schemastore.org/tsconfig",
  "_version": "20.1.0",

  "compilerOptions": {
    "lib": ["es2023"],
    "module": "node16",
    "target": "es2022",
    "strict": true,
    "esModuleInterop": true,
    "skipLibCheck": true,
    "moduleResolution": "node16"
  }
}

package.json

{
  "name": "hello-node-typescript",
  "version": "1.0.0",
  "description": "Hello LaunchDarkly for Node.js with TypeScript",
  "main": "index.ts",
  "scripts": {
    "start": "ts-node ./index.ts"
  },
  "author": "LaunchDarkly <[email protected]>",
  "license": "Apache-2.0",
  "devDependencies": {
    "@types/node": "*",
    "ts-node": "*",
    "typescript": "*"
  },
  "dependencies": {
    "@launchdarkly/node-server-sdk": "../js-core/packages/sdk/server-node",
    "@launchdarkly/js-server-sdk-common": "../js-core/packages/shared/sdk-server",
    "@launchdarkly/js-sdk-common": "../js-core/packages/shared/common"
  }
}

Code:

import { init } from '@launchdarkly/node-server-sdk';
import type { LDSingleKindContext, LDLogger, LDOptions } from '@launchdarkly/node-server-sdk';

// Set sdkKey to your LaunchDarkly SDK key.
const sdkKey = "";

function showMessage(s: string) {
  console.log("*** " + s);
  console.log("");
}

const logger: LDLogger = {
  debug: console.debug,
  info: console.info,
  warn: console.warn,
  error: console.error,
};
const options: LDOptions = {
  logger
};

const client = init(sdkKey, options);

client.once('ready', async function () {
  showMessage("SDK successfully initialized!");
  const context: LDSingleKindContext = {kind: 'user', key: 'bob'};
  const res = await client.variation('my-boolean-flag', context, false);
  console.log("The result", res);
});

This uses a few types, such as the context, logger, and options to ensure they are resolving correctly.

@kinyoklion kinyoklion requested a review from a team as a code owner October 16, 2024 23:09
kinyoklion
kinyoklion commented Oct 16, 2024
View reviewed changes
packages/shared/common/package.json
"types": "./dist/index.d.ts",
"require": "./dist/index.cjs",
"import": "./dist/index.mjs"
"require": { "types": "./dist/cjs/index.d.ts", "default": "./dist/cjs/index.cjs"},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CJS and ESM have to be in different folders in order for this approach to work.

kinyoklion
kinyoklion commented Oct 16, 2024
View reviewed changes
packages/shared/common/tsconfig.json
@@ -13,8 +13,7 @@
"sourceMap": true,
"declaration": true,
"declarationMap": true, // enables importers to jump to source
"stripInternal": true,
"composite": true
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prevents a caching issue and we don't need the composite anymore.

kinyoklion
kinyoklion commented Oct 16, 2024
View reviewed changes
packages/shared/sdk-client/src/DataManager.ts
@@ -9,7 +9,6 @@ import {
ProcessStreamResponse,
subsystem,
} from '@launchdarkly/js-sdk-common';
import { LDStreamProcessor } from '@launchdarkly/js-sdk-common/dist/api/subsystem';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import should not have been allowed.

@kinyoklion kinyoklion mentioned this pull request Oct 16, 2024
Closed
@kinyoklion kinyoklion merged commit 2d2accd into main Oct 17, 2024
22 checks passed
@kinyoklion kinyoklion deleted the rlamb/esm-node-fixes branch October 17, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tanderson-ld tanderson-ld tanderson-ld approved these changes

@louis-launchdarkly louis-launchdarkly louis-launchdarkly approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kinyoklion @louis-launchdarkly @tanderson-ld